Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@model for route templates #523

Merged
merged 4 commits into from Aug 30, 2019
Merged

@model for route templates #523

merged 4 commits into from Aug 30, 2019

Conversation

chancancode
Copy link
Member

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I'm enthusiastic about movement in this direction 🎉 but I have a couple notes/questions about the details as proposed here.

model property on the controller is considered a bad practice and is not really
supported (e.g. `route.modelFor` would return the "wrong" value).

## How we teach this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is one very important unanswered pedagogical question here: what is this template? Is it or is it not a component? (It seems like it is not a component, but something akin to a component.)

If we simply teach it as "route templates are just a special kind of template-only component which receive @model as an argument", then the teaching model around controllers gets weirder in the timeframe between when we ship this and when we replace controllers with a true component-based model. Maybe that's fine, but we should make explicit here IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are (in implementation terms, at least) "outer-html" components whose context/instance is the controller. You can think of Ember.Controller as a special kind of custom components with weird lifecycle semantics (they are singletons).

Whether we want to teach them like that, or draw attention to that fact, is a separate question, but I do think the idea of "route templates are components" are internally consistent, because that's how they are actually implemented.

I think the singleton aspect is indeed weird, but really, that's weird no matter what (whether you think of them as components or not), and I think most of us agree we should figure out how to remove that "feature" sooner rather than later anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very much agreed on the overall direction and implementation angle; my concern is solely on the pedagogical front here.

You can think of a route template as being a special form of component. If it has no backing class, it is just like any other template-only component, and it receives a @model argument.

If it has a backing class, that class is a Controller instead of a Component. We'll cover those details in the Controllers section…

Something like that probably gets the job done, and I think it's probably fine in the interim. 🤔 (Not suggesting that for actual text, to be clear, just thinking out loud.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine. In general, I think we don't really want/need to teach template-only components as a different thing than a "regular" component. It's more like "at its core, a component is just a named piece of template" -> "if you need behavior, you can add a JS file". I think that's applicable here too.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Template only components don’t add the wrapping element though right? Just like the route templates will add no wrapping element.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. However, neither do Glimmer components, which will be the default by the time this is implemented—both in that the feature flag for template-only-components behaving this way will be enabled by default and that @glimmer/component has outerHTML semantics and also will be enabled by default for Octane apps.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webark @chriskrycho going forward, I think this is a better way to think about it:

  1. At its very core, a component is just a named piece of template, and "what you type (in the template) is what you get (when invoking the component)". So, components, in general, are "outerHTML".

  2. Sometimes (and maybe quite often), your component needs to be more than just presentational, so just a piece of template doesn't cut it in those cases. They need behavior, and in those cases, you can add a class for them.

  3. Most of the time, the class should simply provides a blank canvas for your JavaScript (behavior/logic) code and mostly get out of your way. @glimmer/component is our attempt at that to provide an intentionally minimal API to get out of your way, and is a good default going forward.

  4. However, Ember's component system is "pluggable". When @glimmer/component is not sufficient, or for whatever reason is not appropriate, you can inherit from a different super class that suits your needs, whether that is @ember/component, @ember/controller, or other custom components provided by an addon.

  5. These super classes are free to provide different APIs than to your class that makes the task at hand easier. For example, @ember/component provides and API for using positional arguments, and @glimmer/component does not.

  6. In additional to just changing the API though, these super classes are allowed to customize other low-level aspects of the rendering logic. For example, @ember/components adds the "wrapper element" around your template, and @ember/controller caches the instances and make them singleton. These examples are not particularly good uses of those low-level capabilities, and are probably undesirable, but nevertheless, these kind of things are what the super class gets to decide for you, which you are opting into when subclassing from them.

So, to recap:

  • Components = Templates (+ a class, sometimes)
  • Templates: what you type is what you get, no wrapper, no magic
  • Component class: subclass from some super class
  • Super class: has special powers

The main thing I wanted to point out is that, the "special behavior" (no wrapper element) is not in @glimmer/component. That should be the behavior, always, for all components. The thing that is special is @ember/component. Template-only components are not a special thing either. They are just the most basic form of a component, a component that happens to not have a class. Since they didn't subclass from anything, they don't get any special behavior either. So no wrapper element also.

The fact that, once upon a time, "template-only" component automatically got an instance of @ember/component was a mistake, and hopefully one that we will soon forget about going forward.


This can also be thought of as a small incremental step in the bigger picture
of reforming route templates and removing controllers from Ember. Specifically,
it moves us a bit closer to the mental model that controllers/route templates
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious how this might interact with any future proposals for routes being able to import components, etc.? Does this in any way fence off paths in that direction or make them more difficult? And likewise if we added some other kind of magic hooks to routes as a few RFCs have proposed—we obviously don't need to design those here, but this has implications for any such RFC in that it would very strongly suggest similar synthesis of arguments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the general idea is that routes should just invoke a component. For example, we may evolve in a direction where routes just have a args hook that resolves into the arguments that should be passed into the component. In which case they would definitely want to be accessible from the template using the named argument syntax. In that world, the model hook and @model could be a special case/default behavior for the more generalized args hook. That is a transition we need to design and plan out anyway, but I do think we would want "things that are passed in from the route" to be treated as component arguments in any possible future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the model hook and @model could be a special case/default behavior for the more generalized args hook.

I think this is the right general direction, but we should consider skipping this interim step, especially since eliminating controllers is on the 2019-2020 roadmap.

I have concerns about further codifying anything related to the model / beforeModel / afterModel hooks. Going straight to a more generalized args hook (or something similar) would allow someone to define @model as an arg if they want without any possible confusion/conflict with the model hooks. And it would allow us to move forward to a controller-free future by using the presence of an args hook to avoid creating an associated controller entirely (while instead allowing for an optional backing component class).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a conflict. Let's say we go with that design/migration path. The presence of a model hook implies the @model argument. If you have opted into some future args thing then it would be up to you to decide what argument(s) to pass to your component.

class UserRoute {
  model(params) {
    return this.store.find(params.userId);
  }
}

...desugars into...

class UserRoute {
  async args(params) {
    let model = await this.store.find(params.userId);
    return { model };
  }
}

Something like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chancancode I agree that's the logical migration path if we go ahead with this.

The question in my mind is whether this approach is worth the temporary incoherence:

  • It allows access to both this.model and @model in the same template (and allows them to diverge).

  • Bridging an "old" and "new" world means that it is less clear from many angles which world you are working in. Looking at a route template with @model means that you won't know whether you have a backing controller or not. You also may not realize that model hooks are in use in any given route instead of args.

An alternative would be to pursue the args approach immediately and allow developers to make a clean break with models and controllers when they are ready. The presence of the args hook could cause a hard error if it coexists with any model hooks. Furthermore, it would be a signal that no controller should be created.

For me the question comes down to whether we can build consensus around an args-like approach quickly. If not, then I would be in favor of merging this as a Plan B. Although I only focused on the drawbacks, I also recognize the benefits.


## Drawbacks

None?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly confusion in the learning model during the transition period, per comments above.

@NullVoxPopuli
Copy link
Sponsor Contributor

NullVoxPopuli commented Aug 7, 2019

Am fan.
Aligns with: #499, so I'm happy :D
(might need some updates tho)

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks great! I have made some inline tweaks and have a couple specific requests:

  • The "How we teach this" is fairly bare, can you update to include a bit more color on the proposed changes to the @ember/routing/route API documentation and to mention at least a few of the major points WRT the guides that are mentioned throughout the motivation section?
  • It has been my experience that it is fairly common to want to use a more domain specific name than model, and folks therefore customize their setupController methods to set alternative properties. This RFC doesn't necessarily need to deal with that case, but I do think it should be mentioned (perhaps in drawbacks? or detailed design as a caveat?).

text/0523-model-argument-for-route-templates.md Outdated Show resolved Hide resolved
text/0523-model-argument-for-route-templates.md Outdated Show resolved Hide resolved
text/0523-model-argument-for-route-templates.md Outdated Show resolved Hide resolved
@wycats
Copy link
Member

wycats commented Aug 15, 2019

I've been working with Octane features quite a bit over the past few months, and I gotta say: this.model feels pretty different than the rest of the design, and not for any good reason.

The way we will teach components post-Octane is that components take arguments, which are accessible through @arg. A component can have an optional JavaScript class, which exposes those arguments as this.args.* and its properties are accessible in the template.

Knowing this, a reasonable expectation for route templates is that they take arguments from somewhere, and those arguments are accessible as @arg. A route's template may not literally be a component according to long-time advanced Ember users, but we can reasonably bet that users will think of a route's template as a somewhat special variation on the components they learned and use.

From this perspective, @model simply means "an argument named model passed in". The fact that it's passed in by Ember rather than literal syntax is more of a property of the routing system than it is a property of the component vs. controller split.

Finally, since Glimmer components have a much smaller surface area, the gap between controllers and components have shrunk substantially (neither has DOM). We can probably predict that users will see controllers as a quirky holdover from the old Ember, but I can't see any reason to force them to understand all of the details of a completely different and older programming model simply for historical reasons.

It is no longer idiomatic to see this.something as an argument. Instead, this.something refers to properties on a template's backing class, and arguments are separated. this.model is the last remaining place where something that is logically an argument is accessed from a template as a property on the backing context. Eliminating that inconsistency is worth the effort.

I also think that we should work on reducing other inconsistencies, either by making changes to controllers or by providing a path without controllers. But I think that this mental model improvement stands alone and makes new code that mixes routes and components feel more consistent.

@balinterdi
Copy link

The way we will teach components post-Octane is that components take arguments, which are accessible through @arg. A component can have an optional JavaScript class, which exposes those arguments as this.args.* and its properties are accessible in the template.

@wycats I think you mean accessible as @args (in plural), so @args.title, @args.onClick, and so on. Right?

@chriskrycho
Copy link
Contributor

I think rather that it'll be @title, @onClick, etc.

@balinterdi
Copy link

balinterdi commented Aug 15, 2019

@chriskrycho You're right of course. What led me astray is that Yehuda said arguments are accessible as @arg and I thought it then needs to be plural, too. I didn't realize that arg is the actual argument name (e.g title). Sorry, carry on :)

@jenweber
Copy link
Contributor

jenweber commented Aug 19, 2019

Edit - I understand much better now why @model makes sense. The main one is that in a route js, you can't do this.model and make changes. The model also doesn't "belong" to the route js (it's the function at that point) or the controller. It's a bit odd to have this.someAttribute vs @model but we should be discouraging keeping state in controllers anyway, aside from Query Params. I also see how this does not block our future work on controllers/routes. It seems to be a good addition/change. I believe it should go into effect after Octane's initial release, to avoid scope creep.

I think this RFC is difficult to evaluate in isolation because it also has design implications on what we can do with controller refactors. It possibly should be part of those controller design RFCs, and not a standalone.

Here's how I see the teaching challenge:

this.model, to me, means "the model for this route." It implies a union between the route template and controller. If you don't use controllers, your mental model draws a dotted line between the model hook and the route template. This understanding kinda works, but also gets confusing once you do need to understand controllers.

@model sort of fixes the problem above, because it signals that something is coming from outside contexts. However, we have to explain controller context upfront, which does not seem in line with where we want to be headed in Ember. The Roadmap RFC has a goal of eliminating controllers.

Given that both of these teaching approaches are confusing, my initial thoughts are that we should hold off, and re-review this after the initial launch of Octane, in concert with RFCs like #499

I'm still working to understand this RFC better, and will update this comment if I get new insights.

@MelSumner
Copy link
Contributor

At first I was a bit wary that this was inextricably linked to future features (and that supporting this change would somehow be seen as an endorsement of those), but after the extra discussion I now understand the reasons and goals in a way that will help me explain it to others when the time comes.

However, I do agree with Jen that we should wait to FCP or merge this until after Octane has shipped, and as part of the roadmap work of "moving away from controllers". That seems a sensible approach from a project perspective.

Thank you for doing this work, and taking the time to explain it more completely. 👍


In some applications, developers have developed a pattern to override the
`setupController` method to assign the model to a different property on the
controller other than the default `model` naming convention.
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chancancode by my reading of the detailed design, "@model will not be updated when this.model is mutated" which implied it is somehow coupled to the return from model() { and not based on the controller's model property.

As such why would the implementation of setupController matter? Shouldn't the return value of model() { always be available at @model regardless of what happens in setupController?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. That is correct. This is referring to that some teams currently prefer to override the hook to set this.post instead of this.model, because they prefer the name. They will still be able to do that, and @model will still contain the right value, they would just have to choose between going back to the model name (@model) or continue to indirect via the controller (this.post), not @post (yet).

@rwjblue
Copy link
Member

rwjblue commented Aug 23, 2019

We discussed this in today's core team meeting and are 👍 on moving this into final comment period.

@rwjblue
Copy link
Member

rwjblue commented Aug 30, 2019

Thanks for the awesome discussion everyone!

@rwjblue rwjblue merged commit ac27abb into master Aug 30, 2019
@rwjblue rwjblue deleted the model-arg branch August 30, 2019 18:36
chancancode added a commit to emberjs/ember.js that referenced this pull request Sep 3, 2019
The key changes are in `route.ts` and `syntax/outlet.ts`.

The rest are just threading through those new values and test changes.
chancancode added a commit to emberjs/ember.js that referenced this pull request Sep 3, 2019
This is an extension to the RFC not explicitly written in the RFC text.

I missed this when writing the RFC, but we felt that `{{mount}}` with
the `model=` argument is even more clearly an argument, and that we
explicitly decided to restrict the `{{mount}}` syntax to a single
`model` argument (as opposed to arbitrary named arguments), so it is
within the spirit of the RFC to make this work also.

This also refactor the implementation of `{{mount}}` to do less custom
work (like diffing the tag) and let Glimmer VM do more of the work via
the usual paths.
chancancode added a commit to emberjs/ember.js that referenced this pull request Sep 3, 2019
The key changes are in `route.ts` and `syntax/outlet.ts`.

The rest are just threading through those new values and test changes.
chancancode added a commit to emberjs/ember.js that referenced this pull request Sep 3, 2019
This is an extension to the RFC not explicitly written in the RFC text.

I missed this when writing the RFC, but we felt that `{{mount}}` with
the `model=` argument is even more clearly an argument, and that we
explicitly decided to restrict the `{{mount}}` syntax to a single
`model` argument (as opposed to arbitrary named arguments), so it is
within the spirit of the RFC to make this work also.

This also refactor the implementation of `{{mount}}` to do less custom
work (like diffing the tag) and let Glimmer VM do more of the work via
the usual paths.
chancancode added a commit to ember-learn/guides-source that referenced this pull request Sep 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet